Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

fix: crypto in insecure browser context #149

Closed
wants to merge 2 commits into from

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Jul 4, 2019

This PR adds crypto-browserify to the dependencies and replaces crypto with crypto-browserify when bundled in the browser.

Please see #105 for more context. TLDR; webcrypto is unavailable in insecure context i.e. not HTTPS or localhost. This effectively makes JS IPFS unusable when accessed in the browser from HTTP.

In files that require webcrypto we check to see if it's available. If it is not we require the Node.js implementation (which has crypto replaced with crypto-browserify) and if it is available then we use the webcrypto version (so we get fast crypto).

Shipping crypto-browserify adds to the bundle size:

Current gzipped size: 142,824 bytes
New gzipped size: 214,499 bytes

Difference: +71,675 bytes

It's not an insignificant addition so we need to decide whether this is worth it.

If not accepted, we need to add checks when libp2p-crypto methods are called and callback with an appropriate error message. JS IPFS will continue to have issues opened with confusion around this otherwise! See ipfs/js-ipfs#963 ipfs/js-ipfs#964 ipfs/js-ipfs#2017 ipfs/js-ipfs#2153

resolves #105
resolves ipfs/js-ipfs#2017
resolves ipfs/js-ipfs#2153

cc @lidel @autonome

This PR adds `crypto-browserify` to the dependencies and replaces `crypto` with `crypto-browserify` when bundled in the browser.

In files that require webcrypto we check to see if it's available. If it is not we require the Node.js implementation (which has `crypto` replaced with `crypto-browserify`) and if it is available then we use the webcrypto version (so we get fast crypto).

Shipping `crypto-browserify` adds to the bundle size:

Current gzipped size: 142,824 bytes
New gzipped size: 214,499 bytes

Difference: **+71,675 bytes**

It's not an insignificant addition so we need to decide whether this is worth it.

If not accepted, we need to add checks when libp2p-crypto methods are called and callback with an appropriate error message. JS IPFS will continue to have issues opened with confusion around this otherwise! See ipfs/js-ipfs#963 ipfs/js-ipfs#964 ipfs/js-ipfs#2153

resolves #105

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@achingbrain
Copy link
Member

I wonder if we couldn't log a bunch of warnings and then pull the big, slow implementation down at runtime if we detect we are running under http? Or use code splitting so as to not punish people who do the right thing?

@alanshaw
Copy link
Member Author

alanshaw commented Jul 4, 2019

Yep, that might be a good optimisation to make as an iteration on this. We'd have to weigh up if not being able to use IPFS in an offline environment is worth it though.

@daviddias
Copy link
Member

daviddias commented Jul 4, 2019

What about:

  • Auto detect if being run in insecure context and log a nice error that informs the users
  • Make the crypto implementation a config option with the default being WebCrypto. That way, users can pass crypto-browserify for non secure origins and solve the issue themselves.

I believe that most of the pain points here are caused due to lack of information (most devs don't know the intricacies of WebCrypto) and have no option to fix it themselves.

@hugomrdias
Copy link
Member

I support David's proposal 100%

@lidel
Copy link
Member

lidel commented Jul 4, 2019

"Secure Context" according to Browser Vendors

FYSA current browser vendor implementation is:

This means js-ipfs is effectively broken on pages loaded from non-HTTPS or non-127.0.0.1 gateways

Examples of "insecure context" issues:

  • localhost gateway at http://localhost is not a secure context(!) – super confusing for devs
    note: to be a Secure Context it needs to be accessed via raw IP
    (http://127.0.0.1:8080 or http://[::1]:8080)
  • IPFS-in-a-box as a HTTP Gateway for trusted/offline LAN won't be a Secure Context
  • Localhost HTTP Proxy running at http://{cid}.ipfs.localhost will be as secure as a regular gateway on 127.0.0.1 but browser won't expose webcrypto on .ipfs.localhost
  • we can't run js-ipfs in DataURL-based ephemeral pages (data:text/html,<h1>hello</h1> is not a Secure Context)

"Meaningful Error" vs "Just Work"

I am personally torn here, but below opinion is formed by where IPFS is right now.

Printing meaningful error messages (multiformats/js-multihashing-async#30) will help some developers, but it is naive to think we won't have issues about this going forward. Dweb developers are unable to fully control how websites are distributes/mirrored/loaded with IPFS, missing webcrypto will continue to break things, and issues will be reported, but this time by third parties using unencrypted gateways in LAN etc.

I don't like the increased bundle size, but imo breaking things in insecure contexts is much worse. Beginners and regular users won't care about Secure Contexts, just like they did not care about intricacies of DHT or IPNS. More technical devs will google the error and find multiple issues opened over and over again. Both cases will contribute data points forming opinion that IPFS is flaky. We should make it "Just Work".

TL;DR I am leaning towards supporting insecure contexts out of the box: we should print warning about degraded performance due to lack of native webcrypto APIs, we can optimize it later and lazy-load crypto-browserify, but it should "Just Work".

@jacobheun
Copy link
Contributor

I agree with @lidel, it just needs to work out of the box. We can look at adding more advanced configurations to lower bundle size for developers, but we need to make sure the barrier to entry is small, even if it bloats the bundle size a little bit.

In regards to the bundle size, does node-forge not give us the crypto support we need in the browser? We seem to have leveraged it less and less, but it's still included as a dependency. There are some other issues that would benefit from us using node-forge for everything, such as #125, so if we just use it, we could potentially keep the bundle size down. I'm going to look more at node-forge and see if there's a gap there, unless someone already knows the limitations.

@vasco-santos
Copy link
Member

I share the same opinion as some folks before. Firstly, it should just work without any extra configurations. Then, advanced users should be able to configure IPFS, in order to reduce their bundle size

@hugomrdias
Copy link
Member

The problem is we can't include crypto-browserify and say advanced users can reduce size with an option, once crypto-browserify it's there it's there.

Dynamic import also not an option for several reasons already discussed in many issues like this one (maybe one day).

Descriptive error messages with a link to documentation are really underrated in this crowd 😆.
This approach has been used forever and React made it the norm for browser modules and it's a better compromise than increasing size and bloat.
We made this compromise for ipld formats ipfs/js-ipfs#1980 and it was a good one.
We should do the same here!

This is obviously a problem, especially the local gateway/api situation, but browsers forced this onto us nothing we can do about it.
Refs:
https://letsencrypt.org/docs/certificates-for-localhost/
https://justmarkup.com/articles/2018-05-31-https-valid-certificate-local-domain/

We should not degrade the experience of the majority of the users for the "just works" dream especially when we have good options.

"Just works" "Works out of the box" from my experience always breaks somewhere, brings bloated code and degrade experience for the end user. On the other hand consistency, descriptive errors, documentation with copy paste solutions makes users feel comfortable and transmits maturity.

@alanshaw
Copy link
Member Author

alanshaw commented Jul 4, 2019

In regards to the bundle size, does node-forge not give us the crypto support we need in the browser? We seem to have leveraged it less and less, but it's still included as a dependency.

This might be a good option. I did a quick look and it seems it already has everything we're using except ECDH. We could use https://github.com/indutny/elliptic for that.

@daviddias
Copy link
Member

daviddias commented Jul 5, 2019

I believe this to be a clear example of values in tension, things such as "Just works" "Works out of the box" "Magical" often collide with "explicit" "maintainability" "clarity" and "robustness".

Given that IPFS's mission is to make a P2P Content Addressed File System for many centuries, I'm inclined to always lean on whatever strategies will enable us to weather future storms (e.g. Clarity on what it is doing, debugability, good documentation, good tests).

Update: To clarify, I'm not advocating to go fully verbose to the point that we exhaust everyone's will tank to the point that they can not use the implementation anymore. It is great to have sane defaults that help the majority of the users and/or that prioritize the use cases for which it was designed. The tricky bit is when there is tension, one needs to know what to prioritize and in those cases, less magic is preferred.

@daviddias
Copy link
Member

Btw, another option is to finally create the js-ipfs-core package, similar to what js-libp2p is, just a harness in which one plugs all the different components and then have js-ipfs to be the "preferred flavor of the IPFS Core developers, but you are still welcome to use ipfs-core if you want to"

@alanshaw
Copy link
Member Author

alanshaw commented Jul 5, 2019

My main concern is getting something done about this ASAP. It's been a very long time to not do anything about this problem and given that it completely prevents people using IPFS or libp2p in the browser without paying for an SSL cert, I think we should bump the priority slightly 😜

I don't understand what's magical or unmaintainable about using a module that is built specifically to enable crypto in the browser with an API that's identical to Node.js. We already have code to interface with Node.js's crypto module - it's a drop in replacement.

Is it magical to have crypto in the browser when in an "insecure context"? I don't feel it is magical since webcrypto hasn't been around forever and people have been bundling crypto-browserify for 3+ years before webcrypto was even standardised.

On the other hand, are the reasons that browsers have decided we can't have webcrypto in insecure contexts relevant to us? I don't know exactly the reasons or exactly what we expose ourselves to if we do enable crypto in the browser, but my gut feeling is that it's a valid reason to not make it just work by default.

Bundle size is a valid concern and is one of the reasons why I dived deeper to investigate this - before we had no idea of the overhead, now we do. 70kB feels like a lot IMHO, but if my ONLY choices are, use HTTPS or increase the bundle size then...I'd go for increase the bundle size every time. I think everyone paying a small price to enable the people who are not privileged with an SSL cert (either by cost, knowledge or experience) is worth it.

Luckily those aren't the only choices, I think the suggestion of passing your own webcrypto implementation is great - I hadn't thought of that (thanks @daviddias). That in combination with a great error message sounds like the most pragmatic solution to me. It seems like the best of both worlds: we don't lock out people using IPFS in insecure contexts, we help them to fix it, we don't have to increase our bundle size AND we don't open ourselves up to any exploits by using crypto in insecure contexts.

@daviddias
Copy link
Member

Another concern is also performance. crypto-browserify is significantly slower than WebCrypto (hence the reason why we moved away from it in the first place. How should we go about communicating to users that "Your thing might run 10x slower if you don't get that SSL cert?", if not communicated clearly and just "resolved by shimming crypto-browserify", you will start having users that report IPFS sometimes being way slower on the browser.

@lidel
Copy link
Member

lidel commented Jul 8, 2019

are the reasons that browsers have decided we can't have webcrypto in insecure contexts relevant to us?

Some time ago browser vendors decided to add new WebAPIs only to Secure Contexts. The goal was to incentivize use of HTTPS. WebCrypto is an example of that. It is also especially sensitive, because if page was not loaded in secure context, the JS code using the API could be modified via MITM by a malicious third party and replaced with one that leaks private keys etc.

For IPFS this matters if a page was loaded over unencrypted HTTP from non-localhost gateway. I think we could display additional PSA warning message if window.isSecureContext is false and window.location.hostname does not point at localhost, informing user about potential for MITM due to the lack of HTTPS.

I think the suggestion of passing your own webcrypto implementation is great [..] That in combination with a great error message sounds like the most pragmatic solution to me.

I agree. This would unlock devs to use IPFS in contexts without webcrypto apis without the side effects pointed out by @hugomrdias.

We may still get issues reported by third parties, but at least there will be a path for them to ask upstream developer to provide a fix (if feasible).

alanshaw pushed a commit that referenced this pull request Jul 9, 2019
Changes `webcrypto.js` to check for native web crypto availability and falls
back to using `window.__crypto` if not available.

If the user wants to bring their own Web Crypto API compatible implementation
then they simply need to assign it to `window.__crypto` before they start using
IPFS.

Checks are done in the functions that require web crypto to give the user the
flexibility to assign to `window.__crypto` before OR after they import
`libp2p-crypto`. It also means that users have the ability to use other exported
functions that do not require web crypto without having to worry about sorting
their own implementation.

We use `window.__crypto` because `window.crypto` is a readonly property in
secure context and always readonly in workers.

If `window.crypto` and `window.__cypto` are unavailable then an appropriate
error message is reported to the user with a `ERR_MISSING_WEB_CRYPTO` code.

I've also added documentation to the README.

This is a backwards compatible change.

closes #149
resolves #105
resolves ipfs/js-ipfs#2017
resolves ipfs/js-ipfs#2153

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@alanshaw alanshaw closed this Jul 10, 2019
@alanshaw alanshaw deleted the fix/crypto-in-unsecure-context branch July 10, 2019 11:14
alanshaw added a commit that referenced this pull request Jul 17, 2019
This PR simply detects missing web crypto and throws an error with an appropriate message.

This is a stepping stone that will help users understand the problem until we have time to do a refactor of this module and of all the modules that use it to enable optionally passing your own crypto implementation.

refs #149
refs #150
refs #105
refs ipfs/js-ipfs#2153
refs ipfs/js-ipfs#2017

License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
jacobheun pushed a commit that referenced this pull request Jul 22, 2019
This PR simply detects missing web crypto and throws an error with an appropriate message.

This is a stepping stone that will help users understand the problem until we have time to do a refactor of this module and of all the modules that use it to enable optionally passing your own crypto implementation.

refs #149
refs #150
refs #105
refs ipfs/js-ipfs#2153
refs ipfs/js-ipfs#2017

License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
Mikerah pushed a commit to ChainSafe/js-libp2p-crypto that referenced this pull request Aug 29, 2019
This PR simply detects missing web crypto and throws an error with an appropriate message.

This is a stepping stone that will help users understand the problem until we have time to do a refactor of this module and of all the modules that use it to enable optionally passing your own crypto implementation.

refs libp2p#149
refs libp2p#150
refs libp2p#105
refs ipfs/js-ipfs#2153
refs ipfs/js-ipfs#2017

License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
7 participants